-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make gcno-files available in CcCompilationOutputs #12129
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
78f4211
to
ad3b7d3
Compare
ad3b7d3
to
61df36e
Compare
We also need this functionality added to bazel. |
You also may want this to be able to pick the correct files in a clean way: #12229 |
ping @oquenchil? |
Is somebody looking at that? |
I agree with the goal of this PR and Bazel needs to allow you to do this. However, can you first try out if you can get to these files through the InstrumentedFilesInfo? If there is any reason why that's not possible, then the way you do it here makes sense. |
Yes. We're doing custom rules using cc_common interface. InstrumentedFilesInfo is not available from there. For these kind of files it for me looks cleaner if the featue or rule specifying the options generating the files also specifies the additional output files. In this case it's tricky though due to the pic/nopic situation and the fact that the rule/feature has no control over object naming. |
While waiting for cc_binary to become Starlark we could also use the map file to select correct gcno file (pic vs nopic), see #12883 how to get this file. |
Why not? Is Bazel not making the API available to you to use InstrumentedFilesInfo? If it's just a matter of taste, I'd go with those custom rules starting to use InstrumentedFilesInfo since that's the mechanism and API that we have. |
I'm using cc_common.compile to create the compile action. It returns no InstrumentedFileInfo. |
Ping @oquenchil
For 1 maybe if the object paths are stable, paths could be hard-coded in rules. 2 could possibly be solved with two arguments, "additional_outputs" and "additional_pic_outputs". Any ideas? |
I've rebased this patch on master (5.0 prereleases), it required updates for gcno-files to show up using the "build" command. Patch on master is here: https://github.com/torgil/bazel/tree/torgil/compile-gcno-output If you're looking for a patch on 4.1.0, see https://github.com/torgil/bazel/tree/torgil/compile-gcno-output-4.1.0 |
b2a7287
to
4f6d55d
Compare
Rebased versions of coverage patches + additional essential patches can be found here: https://github.com/torgil/bazel/commits/master |
@torgil, I'm going to go ahead and close this PR, because it seems to have stalled. If you're still interested in pursing this (and responding to my comments), please feel free to reopen! Thank you! |
The gcno-files are generated during compilation (not during test) and are already available in CcCompilationOutputs. They are just not accessible from Starlark which is what this patch do. Bazel test/coverage also runs outside the normal build tree which makes it not useful if the information is needed as input to another higher level target. |
I first thought that an "additional_outputs" argument to cc_common.compile similar to cc_common.link should be more generic and useful but were discouraged due to the pic/nopic situation. It it possible to have both pic and nopic result from a single cc_compile call? |
I talked with someone over the phone at NYC Bazel Con that said an "additional_outputs" option was feasible in conjunction with some way to detect if a binary was linked with pic or nopic. That would be a more generic solution to this problem and would cover all kinds of files (analog to "additional_outputs" in cc_common.link) like stack usage files and dependency files (for which we currently have another custom patch). Is anyone following up on this? |
Hey torgil that was me. I will prioritize this for after the holiday break. Will be working together with nikhilkalige who filed this issue: #15924 Completely different problem but I think we can arrive at a versatile solution that allows people to get different files and flags through for their specific use cases without needing to pipe them via Bazel. After the break I will describe what the solution could look like, will group many issues together like this one that could benefit from it and nikhilkalige will take it on from there. |
Created #17237 |
After looking at this pull request again and the issue that I created. I'm thinking now that it doesn't really fit. The issue here is for gcno files whose output artifact is already created by Bazel, this is about piping them through CcCompilationOutputs to custom coverage rules. I didn't see the mismatch until having them side by side. I don't want to delay you any longer, if you rebase this change I will merge it. |
Thanks. Awesome. I have a few followup questions:
|
Is there a compiler flag that lets you customize the suffix of the gcda file? If that's the case we can accept a PR that adds support for this in Bazel and the default toolchain. The code would add the suffix *.pic.gcda for .pic.o. If not, I need to know more details about how the custom coverage rule is implemented. Does it depend directly on a cc_binary? How are you getting to the object files, is it with an aspect? If it depends directly on the cc_binary, what you can do is to have an aspect (that doesn't propagate through attributes, just one level) on the custom coverage rule applied to the cc_binary dependency. The aspect can look at the target's actions, get to the linking action and get the list of inputs, here you would build a list of paths from the inputs (all of it in analysis). Later when iterating over all the objects in analysis as you were doing, you can check if its path is on the list (or dictionary) that you built. Related that it might be of interest to you is that in the past month internally we decided to just build everything as PIC when not in production. See commit 973f6258c6, the mock toolchain shows the feature that you'd have to add to your toolchain or to the default Bazel one which doesn't have it yet. If this is only for coverage you may as well just build everything with PIC. |
This starts to get rather complex as it needs modifications to
Maybe both step 1 and step 2 could be removed if the coverage action get access to the map-file from the binary linking step in which it can see object file paths and match it with gcno file paths. Still quite complex compared to accessing pic/nopic state in LinkingOutputs.
It has access to the output of cc_common.link
The outputs of cc_common.compile is fed to cc_common.link through CcCompilationOutputs (which this PR wants more information from)
Can this be done directly after the call to cc_common.link in the rule producing the binary as well ?
I will not always have control over all toolchains (downstream) so I prefer not to add that dependency. |
could you rebase your change? it looks like some of the fields you're adding are already present |
This is needed to do custom code coverage rules.
98b5967
to
362dc09
Compare
Rebase fails as this functionality already implemented behind a private API.
I remade the patch to remove the private api check instead. I'm not sure how to do with the "enableCoverage" check, I just removed it for now as it's more tided to "bazel coverage" command than the "coverage" feature. With current patch It'll generate an empty gcno file if coverage feature is not enabled which may not be optimal. |
We can't have this. I understood this PR to just be a refactoring to allow you to get to the files. Why not leave the
I agree with this but now that it's public API, could you please add a test to StarlarkCcCommonTest? |
Reviewed old code. Currently discussing new changes.
Any updates on this? |
This is needed to do custom code coverage rules.